fix(harness): compare the /ingest bearer token in constant time - #658
Open
memosr wants to merge 1 commit into
Open
fix(harness): compare the /ingest bearer token in constant time#658memosr wants to merge 1 commit into
memosr wants to merge 1 commit into
Conversation
Every other boot-token check uses timingSafeEqualString: createBootTokenMiddleware guards the whole /api surface, and the WS upgrade path guards events-ws. Its doc comment states the reason, and it even balances the length-mismatch branch so that case does not resolve faster. POST /ingest was the one path left on a plain !== against the same secret. The server binds to 127.0.0.1, but that is precisely the threat model here: the harness runs on a machine where a coding agent executes arbitrary commands, so a local process that can reach /ingest but does not hold the token could recover it byte by byte and then use it against all of /api. Use the existing helper. Also cover the missing-header case, which was untested.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Primary change type
Problem and motivation
POST /ingestchecks the boot token with a plain string comparison:Every other check of that same secret already runs in constant time:
createBootTokenMiddleware(server/auth.ts), which guards the whole/apisurfaceserver/events-ws.ts)timingSafeEqualStringeven balances its length-mismatch branch on purpose, and its doc comment states why./ingestis the one path left out, anddeps.ingestTokenis the sameoptions.bootTokenthe other two protect.The server binds to
127.0.0.1, so this is not remotely reachable. The change brings one path in line with the package's own established pattern.Summary and scope
Use the existing helper. One import, one condition, no new dependency, and no behavior change for valid or invalid tokens.
Out of scope: the auth helper itself and the other two call sites are already correct and are not touched.
Related work
Related issue or discussion: N/A. See the Security section below — I would like maintainer guidance on whether this should have gone through the Security Policy instead.
Validation
Tests and documentation
Tests: added a case for a request with no
Authorizationheader at all, which was untested and is the path the?? ""now covers. The existing wrong-token case already exercises a token of a different length.Documentation: N/A, no user-facing behavior changed.
Compatibility and release impact
@sapiom/harness).Security
I want to be straightforward about the unchecked box rather than quietly tick it. I judged this as hardening of a local-only path that simply missed the pattern already applied to
/apiand the WS upgrade, so I opened it directly. Re-reading CONTRIBUTING.md ("even if it looks like a bug fix"), I am no longer confident that was the right call.Nothing here is novel: the fix is a one-line use of a helper that already exists in the repository for exactly this purpose, and the gap is visible to anyone reading the two files side by side. But if you consider this in scope for the Security Policy, say so and I will close this PR and send it to team [at] sapiom.ai instead. I will follow the private channel for anything less clear-cut in future.
AI assistance
I used Claude (Claude Code and the Claude app). Claude reviewed the package and reported that
/ingestwas the only boot-token check not usingtimingSafeEqualString; I confirmed this by readingauth.ts,events-ws.ts,ingest.tsand the threeingestToken: options.bootTokencall sites inserver/index.tsmyself. I directed the fix and the added test case, and Claude applied them. I ran the full server suite and the type and lint checks and reviewed the diff before pushing.Checklist
CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.